module: support loading entrypoint as url #54933

RedYetiDev · 2024-09-13T22:28:32Z

Ref: #49975
Fixes?: #46009
Fixes?: #49204

The original PR stalled, so I addressed the comments, and reimplemented it in a way that it works with the latest Node.js codebase.

nodejs-github-bot · 2024-09-13T22:28:37Z

Review requested:

@nodejs/loaders

codecov · 2024-09-13T23:51:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.24%. Comparing base (75e4d0d) to head (0226b30).
Report is 180 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54933      +/-   ##
==========================================
+ Coverage   88.06%   88.24%   +0.17%     
==========================================
  Files         651      651              
  Lines      183451   184539    +1088     
  Branches    35821    35885      +64     
==========================================
+ Hits       161564   162852    +1288     
+ Misses      15133    14948     -185     
+ Partials     6754     6739      -15

Files with missing lines	Coverage Δ
lib/internal/main/run_main_module.js	`100.00% <100.00%> (ø)`
lib/internal/modules/run_main.js	`97.31% <100.00%> (+0.02%)`	⬆️
src/node_options.cc	`88.06% <100.00%> (+0.03%)`	⬆️
src/node_options.h	`98.25% <100.00%> (+0.04%)`	⬆️

... and 206 files with indirect coverage changes

doc/api/cli.md

LiviaMedeiros

The code LGTM.

Adding more tests (and maybe examples in the docs) would be appreciated, and i think it should be more clear that --entry-url is a binary flag that doesn't require --entry-url <URL> syntax, i.e. node --any-flags file:///file.mjs --any-flags --entry-url --any-flags also works.

Also, IIRC src subsystem exists mainly for generic changes (just like lib), so just module should be sufficient.

test/es-module/test-esm-loader-entry-url.mjs

lib/internal/modules/run_main.js

test/es-module/test-esm-loader-entry-url.mjs

RedYetiDev · 2024-09-21T15:13:51Z

If someone could start a CI (I can't without a newer approval), it would be appreciated.

nodejs-github-bot · 2024-09-21T16:39:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62644/

RedYetiDev · 2024-09-22T13:08:01Z

Looks like some of the jobs were stopped, can someone resume them?

RedYetiDev · 2024-09-26T16:38:43Z

node/lib/internal/modules/run_main.js

Line 35 in 51138ac

mainPath = path.resolve(main);

Found it. It looks like to fix this, when --entry-url is supplied, the path shouldn't be resolved.

lib/internal/modules/run_main.js

nodejs-github-bot · 2024-09-26T18:02:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62787/

nodejs-github-bot · 2024-09-26T23:06:49Z

Commit Queue failed

- Loading data for nodejs/node/pull/54933 ✔ Done loading data for nodejs/node/pull/54933 ----------------------------------- PR info ------------------------------------ Title module: support loading entrypoint as url (#54933) Author Aviv Keller <redyetidev@gmail.com> (@RedYetiDev) Branch RedYetiDev:entry-url -> nodejs:main Labels module, semver-minor, cli, author ready, needs-ci Commits 2 - module: support loading entrypoint as url - fixup! Don't resolve `main` when `--entry-rul` Committers 2 - RedYetiDev <38299977+RedYetiDev@users.noreply.github.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/54933 Refs: https://github.com/nodejs/node/pull/49975 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54933 Refs: https://github.com/nodejs/node/pull/49975 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 13 Sep 2024 22:28:32 GMT ✔ Approvals: 5 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54933#pullrequestreview-2304325037 ✔ - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/54933#pullrequestreview-2331961041 ✔ - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/54933#pullrequestreview-2322942162 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54933#pullrequestreview-2323247699 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/54933#pullrequestreview-2332482818 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-09-26T18:02:10Z: https://ci.nodejs.org/job/node-test-pull-request/62787/ - Querying data for job/node-test-pull-request/62787/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 54933 From https://github.com/nodejs/node * branch refs/pull/54933/merge -> FETCH_HEAD ✔ Fetched commits as 668e52339261..0226b3041e16 -------------------------------------------------------------------------------- Auto-merging doc/api/cli.md Auto-merging lib/internal/modules/run_main.js Auto-merging src/node_options.cc Auto-merging src/node_options.h [main c182ee2694] module: support loading entrypoint as url Author: RedYetiDev <38299977+RedYetiDev@users.noreply.github.com> Date: Fri Sep 13 18:26:17 2024 -0400 8 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 test/es-module/test-esm-loader-entry-url.mjs create mode 100644 test/fixtures/es-modules/print-entrypoint.mjs Auto-merging lib/internal/modules/run_main.js [main fb8241ca63] fixup! Don't resolve `main` when `--entry-rul` Author: Aviv Keller <redyetidev@gmail.com> Date: Thu Sep 26 12:41:46 2024 -0400 1 file changed, 9 insertions(+), 3 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
module: support loading entrypoint as url

Co-Authored-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #54933
Refs: #49975
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

[detached HEAD a4bb406bfd] module: support loading entrypoint as url
Author: RedYetiDev <38299977+RedYetiDev@users.noreply.github.com>
Date: Fri Sep 13 18:26:17 2024 -0400
8 files changed, 140 insertions(+), 7 deletions(-)
create mode 100644 test/es-module/test-esm-loader-entry-url.mjs
create mode 100644 test/fixtures/es-modules/print-entrypoint.mjs
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! Don't resolve main when --entry-rul

PR-URL: #54933
Refs: #49975
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

[detached HEAD 27e298ccfd] fixup! Don't resolve main when --entry-rul
Author: Aviv Keller <redyetidev@gmail.com>
Date: Thu Sep 26 12:41:46 2024 -0400
1 file changed, 9 insertions(+), 3 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/11061558581

Co-Authored-By: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #54933 Refs: #49975 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

LiviaMedeiros · 2024-09-27T07:28:13Z

Landed in 772b35b

Co-Authored-By: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #54933 Refs: #49975 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 src: * mark node `--run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: TODO

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 src: * mark node `--run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 src: * mark `node --run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 process: * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241 src: * mark `node --run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

Notable changes: crypto: * (SEMVER-MINOR) add `KeyObject.prototype.toCryptoKey` (Filip Skokan) #55262 * (SEMVER-MINOR) add Date fields for `validTo` and `validFrom` (Andrew Moon) #54159 doc: * add abmusse to collaborators (Abdirahim Musse) #55086 http2: * (SEMVER-MINOR) expose `nghttp2_option_set_stream_reset_rate_limit` as an option (Maël Nison) #54875 lib: * (SEMVER-MINOR) propagate aborted state to dependent signals before firing events (jazelly) #54826 module: * (SEMVER-MINOR) support loading entrypoint as url (RedYetiDev) #54933 * (SEMVER-MINOR) implement the `"module-sync"` exports condition (Joyee Cheung) #54648 * (SEMVER-MINOR) implement `flushCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) throw when invalid argument is passed to `enableCompileCache()` (Joyee Cheung) #54971 * (SEMVER-MINOR) write compile cache to temporary file and then rename it (Joyee Cheung) #54971 process: * (SEMVER-MINOR) add `process.features.require_module` (Joyee Cheung) #55241 * (SEMVER-MINOR) add `process.features.typescript` (Aviv Keller) #54295 src: * mark `node --run` as stable (Yagiz Nizipli) #53763 test_runner: * (SEMVER-MINOR) support custom arguments in `run()` (Aviv Keller) #55126 * (SEMVER-MINOR) add `'test:summary'` event (Colin Ihrig) #54851 * (SEMVER-MINOR) add support for coverage via `run()` (Chemi Atlow) #53937 worker: * (SEMVER-MINOR) add `markAsUncloneable` api (Jason Zhang) #55234 PR-URL: #55343

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Sep 13, 2024

RedYetiDev mentioned this pull request Sep 13, 2024

module,src: add --experimental-entry-url flag #49975

Closed

RedYetiDev added semver-minor PRs that contain new features and should be released in the next minor version. cli Issues and PRs related to the Node.js command line interface. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Sep 13, 2024

jasnell reviewed Sep 14, 2024

View reviewed changes

doc/api/cli.md Show resolved Hide resolved

jasnell approved these changes Sep 14, 2024

View reviewed changes

RedYetiDev force-pushed the entry-url branch from 4b36a9e to 9d90d6c Compare September 14, 2024 00:15

LiviaMedeiros approved these changes Sep 14, 2024

View reviewed changes

test/es-module/test-esm-loader-entry-url.mjs Outdated Show resolved Hide resolved

test/es-module/test-esm-loader-entry-url.mjs Outdated Show resolved Hide resolved

RedYetiDev force-pushed the entry-url branch from 9d90d6c to bebf291 Compare September 14, 2024 15:44

RedYetiDev changed the title ~~module,src: support loading entrypoint as url~~ module: support loading entrypoint as url Sep 14, 2024

RedYetiDev force-pushed the entry-url branch from bebf291 to 8233681 Compare September 14, 2024 15:45

RedYetiDev mentioned this pull request Sep 15, 2024

Data URLs can't have query params #54944

Closed